Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Crash when 3D Tiles bounding region has 0 volume #7945

Merged
merged 16 commits into from
Sep 19, 2019
Merged

fix: Crash when 3D Tiles bounding region has 0 volume #7945

merged 16 commits into from
Sep 19, 2019

Conversation

verybigzhouhai
Copy link
Contributor

No description provided.

Zhouhai added 4 commits June 18, 2019 21:09
Avoid creating a bounding box with a volume equal to 0
Avoid creating a bounding region with a volume equal to 0
Avoid creating a bounding sphere with a volume equal to 0
@cesium-concierge
Copy link

Thanks for the pull request @verybigzhouhai!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@verybigzhouhai verybigzhouhai changed the title fix fix: Crash when 3D Tiles bounding region has 0 volume Jun 18, 2019
@verybigzhouhai
Copy link
Contributor Author

fixed #7933

@verybigzhouhai
Copy link
Contributor Author

verybigzhouhai commented Jun 18, 2019

@OmarShehata @lilleyse To solve this problem, I added validation when creating shpere, box.

fix create region bug
@OmarShehata
Copy link
Contributor

Thanks for opening this PR @verybigzhouhai ! I'll try to take a deeper look soon. For now, can you add a unit test as well to ensure that it does not crash for this type of data in the future?

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata Okay, there are still some problems with the validation of creating regions, and I'll add a unit test after solving them.

Zhouhai added 3 commits June 19, 2019 14:20
another way to solve this problem
add reference and unit test
modify unit test
@verybigzhouhai
Copy link
Contributor Author

this is ready,eventually I added validation for box and shpere. @OmarShehata

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata Will you review this?

@OmarShehata OmarShehata self-requested a review June 25, 2019 02:45
@OmarShehata
Copy link
Contributor

I'll try to take a look sometime this week.

@cesium-concierge
Copy link

Thanks again for your contribution @verybigzhouhai!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata Is this hopefully merged in the next release version?

@OmarShehata
Copy link
Contributor

@verybigzhouhai I think this looks good. The only thing I would change is the name of the test:

can`t have an oriented bounding box with 0 volume

To maybe something like:

does not crash for bounding box with 0 volume

Other than that, @lilleyse do you want to confirm that this was the fix you had in mind for:

I would also make sure that the axes are still orthogonal to each other when one or two of the half-axes are non-zero length.

change name of the test
@verybigzhouhai
Copy link
Contributor Author

@OmarShehata I have modified the name.

@cesium-concierge
Copy link

Thanks again for your contribution @verybigzhouhai!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

Thanks @verybigzhouhai! I pushed a change to the halfAxes check in TileOrientedBoundingBox because the axes need to be orthogonal.

@OmarShehata or @IanLilleyT can you review?

@OmarShehata
Copy link
Contributor

Thanks for making this more robust Hannah. I updated the spec which had pretty low coverage for all the new combinations that checkHalfAxes was checking. It should now be 100% covered.

I spoke to @lilleyse briefly about this last night - I'd still like to get your blessing before merging that this is both necessary and appropriate since it was your original suggestion. From my side, this looks good, and since it exits early from the common case (and seems to only run on tile creation) I think it shouldn't cause any performance dips.

@lilleyse
Copy link
Contributor

@OmarShehata it looks like the test if failing.

I'm good with the approach here. Just want to add that the check will also happen when the tile transform changes but that's not much of a concern either.

@lilleyse
Copy link
Contributor

Also CHANGES.md needs to be updated.

@OmarShehata
Copy link
Contributor

Thanks Sean! I had a typo in the test, my bad. This should be all good now.

Thanks @verybigzhouhai for finding the issue and contributing this fix!

@OmarShehata OmarShehata merged commit 1a16eb9 into CesiumGS:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants